Improve the readability of `List<T>`. by nnethercote · Pull Request #91617 · rust-lang/rust

nnethercote · 2021-12-07T02:58:04Z

This commit does the following.

Expands on some of the things already mentioned in comments.
Describes the uniqueness assumption, which is critical but wasn't
mentioned at all.
Rewrites empty() into a clearer form, as provided by Daniel
Henry-Mantilla on Zulip.
Reorders things slightly so that more important things
are higher up, and incidental things are lower down, which makes
reading the code easier.

danielhenrymantilla

Nice, I like very much the fact that it points out that interning is (expected) to be provided at an outer layer, even though this type expects such semantics 👌

danielhenrymantilla · 2021-12-07T12:13:28Z

compiler/rustc_middle/src/ty/list.rs

To expand on what I mentioned on Zulip, note here that data and opaque are deeply intertwined; having them laid out in such a bare fashion is thus an itsy bit error-prone. That is, compare the current definition, even with your great comments added to it, to:

#[repr(C)] pub struct List<T> { len: usize, /// `len` elements are expected to be present. data: ThinDst<[T]>, } /// Const-assert that a pointer to a `List<T>` is thin. const _: () = { let _ = ::core::mem::transmute::<&List<u8>, usize>; };

Playground

Are you happy with this part of the code as is, or are you suggesting that I change it to what's in the Playground link?

I can't claim which one is better in a vacuum: I like my suggestion since it reads quite well at the level of the definition of List<T>, but now it's the definition of ThinDst which may seem a bit too "messy" for compiler code. So just let it be, unless other people were to chime in in defense of the ThinDst helper

compiler/rustc_middle/src/ty/list.rs

nnethercote · 2021-12-07T21:46:42Z

Thanks for the good comments, I have addressed them and updated the code.

lcnr

some additional small nits, deal with them as you like.

after that r=me

compiler/rustc_middle/src/ty/list.rs

This commit does the following. - Expands on some of the things already mentioned in comments. - Describes the uniqueness assumption, which is critical but wasn't mentioned at all. - Rewrites `empty()` into a clearer form, as provided by Daniel Henry-Mantilla on Zulip. - Reorders things slightly so that more important things are higher up, and incidental things are lower down, which makes reading the code easier.

nnethercote · 2021-12-09T04:21:57Z

New code is up. Gotta be close to an r+ by now 😀

lcnr · 2021-12-09T12:17:48Z

@bors r+

bors · 2021-12-09T12:17:49Z

📌 Commit 769a707 has been approved by lcnr

…y, r=lcnr Improve the readability of `List<T>`. This commit does the following. - Expands on some of the things already mentioned in comments. - Describes the uniqueness assumption, which is critical but wasn't mentioned at all. - Rewrites `empty()` into a clearer form, as provided by Daniel Henry-Mantilla on Zulip. - Reorders things slightly so that more important things are higher up, and incidental things are lower down, which makes reading the code easier. r? `@lcnr`

…y, r=lcnr Improve the readability of `List<T>`. This commit does the following. - Expands on some of the things already mentioned in comments. - Describes the uniqueness assumption, which is critical but wasn't mentioned at all. - Rewrites `empty()` into a clearer form, as provided by Daniel Henry-Mantilla on Zulip. - Reorders things slightly so that more important things are higher up, and incidental things are lower down, which makes reading the code easier. r? ``@lcnr``

…y, r=lcnr Improve the readability of `List<T>`. This commit does the following. - Expands on some of the things already mentioned in comments. - Describes the uniqueness assumption, which is critical but wasn't mentioned at all. - Rewrites `empty()` into a clearer form, as provided by Daniel Henry-Mantilla on Zulip. - Reorders things slightly so that more important things are higher up, and incidental things are lower down, which makes reading the code easier. r? ```@lcnr```

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#91617 (Improve the readability of `List<T>`.) - rust-lang#91640 (Simplify collection of in-band lifetimes) - rust-lang#91682 (rustdoc: Show type layout for type aliases) - rust-lang#91711 (Improve `std::iter::zip` example) - rust-lang#91717 (Add deprecation warning for --passes) - rust-lang#91718 (give more help in the unaligned_references lint) - rust-lang#91782 (Correct since attribute for `is_symlink` feature) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned lcnr Dec 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 7, 2021

danielhenrymantilla approved these changes Dec 7, 2021

View reviewed changes

lcnr reviewed Dec 7, 2021

View reviewed changes

compiler/rustc_middle/src/ty/list.rs Outdated Show resolved Hide resolved

nnethercote force-pushed the improve-List-readability branch from b7dbdb6 to a1628a0 Compare December 7, 2021 21:46

This comment has been minimized.

Sign in to view

nnethercote force-pushed the improve-List-readability branch from a1628a0 to e73ce93 Compare December 7, 2021 22:48

lcnr approved these changes Dec 8, 2021

View reviewed changes

compiler/rustc_middle/src/ty/list.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/ty/list.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/ty/list.rs Outdated Show resolved Hide resolved

nnethercote force-pushed the improve-List-readability branch from e73ce93 to 769a707 Compare December 9, 2021 04:21

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 9, 2021

matthiaskrgr mentioned this pull request Dec 9, 2021

Rollup of 6 pull requests #91712

Closed

matthiaskrgr mentioned this pull request Dec 10, 2021

Rollup of 7 pull requests #91727

Closed

matthiaskrgr mentioned this pull request Dec 11, 2021

Rollup of 6 pull requests #91788

Closed

matthiaskrgr mentioned this pull request Dec 11, 2021

Rollup of 7 pull requests #91792

Merged

bors merged commit 1de7815 into rust-lang:master Dec 11, 2021

rustbot added this to the 1.59.0 milestone Dec 11, 2021

nnethercote mentioned this pull request Feb 4, 2022

Overhaul interning rust-lang/compiler-team#481

Closed

3 tasks

nnethercote deleted the improve-List-readability branch February 7, 2026 03:17

Uh oh!

Conversation

nnethercote commented Dec 7, 2021

Uh oh!

danielhenrymantilla left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhenrymantilla Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

nnethercote Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

danielhenrymantilla Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nnethercote commented Dec 7, 2021

Uh oh!

This comment has been minimized.

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nnethercote commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lcnr commented Dec 9, 2021

Uh oh!

bors commented Dec 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

danielhenrymantilla left a comment •

edited

Loading

nnethercote commented Dec 9, 2021 •

edited

Loading